Skip to content

[PM-33569] feat: Handle displaying login request for alert style notification#2482

Merged
matt-livefront merged 3 commits intomainfrom
matt/PM-33569-auth-request-notification-handling
Mar 26, 2026
Merged

[PM-33569] feat: Handle displaying login request for alert style notification#2482
matt-livefront merged 3 commits intomainfrom
matt/PM-33569-auth-request-notification-handling

Conversation

@matt-livefront
Copy link
Copy Markdown
Collaborator

@matt-livefront matt-livefront commented Mar 24, 2026

🎟️ Tracking

PM-33569

📔 Objective

This adds support for receiving alert-style push notifications for login with device.

In the current approach, the backend sends a silent push notification. The app receives this notification in the background and schedules a local notification for the user to see.

When switching to alert-style push notifications, the notification extension (#2444) will first receive the notification and update the contents of the notification. This alert will display automatically. But there's two changes that are needed to support this flow:

  • The local notification shouldn't be displayed for the alert-style notification or the user will see duplicate notifications.
  • Because the data in the payload of the notification is different, there needs to be a separate path in the app to handle tapping on the alert-style notification.

@matt-livefront matt-livefront requested a review from a team as a code owner March 24, 2026 20:58
@matt-livefront matt-livefront added ai-review Request a Claude code review t:feature labels Mar 24, 2026
@github-actions github-actions bot added the app:password-manager Bitwarden Password Manager app context label Mar 24, 2026
@claude
Copy link
Copy Markdown

claude bot commented Mar 24, 2026

Claude finished @matt-livefront's task in 2m 17s —— View job


Code Review: PM-33569 Handle displaying login request for alert style notification

  • Gather context and review PR changes
  • Review PushNotificationData.swift changes
  • Review NotificationService.swift changes
  • Review NotificationServiceTests.swift changes
  • Post review findings

Summary

This PR cleanly handles the transition from silent push notifications (with local banners) to alert-style push notifications for login-with-device requests. The implementation is well-structured and covers all the necessary paths.

No actionable findings identified. The code looks good:

  • PushNotificationData.swift — Added optional id field to LoginRequestPushNotification to support the alert-style notification payload. Clean and minimal.
  • NotificationService.swift — Core logic changes are well-organized:
    • isAlertNotification parameter properly gates local notification creation to prevent duplicates
    • New alert notification tap handling in handleNotificationTapped correctly parses the different payload format
    • Same-account login request display logic is properly shared between both notification paths
    • Error handling and guard clauses are appropriate
  • NotificationServiceTests.swift — Comprehensive test coverage for all new paths: alert notification receipt, alert notification taps for same/different accounts, and local notification taps for same account.

TODOs are properly tagged with the follow-up ticket (PM-33817) for removing the legacy local notification approach. 👍

@github-actions
Copy link
Copy Markdown
Contributor

Logo
Checkmarx One – Scan Summary & Details0ad80e61-4418-4f95-8051-a18362764dfd

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 24, 2026

Codecov Report

❌ Patch coverage is 98.20359% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.89%. Comparing base (e6d1ebd) to head (4ef7db1).
⚠️ Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
...d/Core/Platform/Services/NotificationService.swift 95.31% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2482    +/-   ##
========================================
  Coverage   86.88%   86.89%            
========================================
  Files        1844     1844            
  Lines      163171   163313   +142     
========================================
+ Hits       141769   141908   +139     
- Misses      21402    21405     +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matt-livefront matt-livefront merged commit 2686997 into main Mar 26, 2026
20 checks passed
@matt-livefront matt-livefront deleted the matt/PM-33569-auth-request-notification-handling branch March 26, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants